-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix incompatible attribute #8
Fix incompatible attribute #8
Conversation
Sorry rebase issue, please hold... |
25b75b4
to
8f32b5d
Compare
This is ready for review by the way (the rebase has been fixed) |
try: | ||
layer_data = handler.to_object(layer_data, | ||
statistic=statistic) | ||
except IncompatibleAttribute: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks awfully similar to glue-viz/glue-astronomy#39 but I think we're patching different things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yes it does seem like the same thing. I think we do want to handle things better in glue-astronomy but I think we still want to raise an error there rather than silently fail, and we do want to catch those exceptions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I would suggest we go ahead with this PR, modify the glue-astronomy one to raise a better exception, and then update the exception we catch in jdaviz once glue-astronomy is released.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works, thank you!
Subsets that can't be computed for a given dataset shouldn't be converted to e.g. Spectrum1D and so on.